feat: Add core definitions for dns-account-01 by sheurich · Pull Request #8140 · letsencrypt/boulder

sheurich · 2025-04-23T20:37:05Z

Summary

This PR introduces the foundational components required to support the dns-account-01 challenge type, as specified in draft-ietf-acme-dns-account-label-00.

Updated Scope (per review feedback): This PR now focuses only on core definitions and SA support. PA/VA/RA logic moved to PR #8149.

Changes

Core Definitions & Logic:

core/objects.go: Added ChallengeTypeDNSAccount01 constant and updated validation methods
core/challenges.go: Added DNSAccountChallenge01 constructor and factory support

Storage Authority (SA) Support:

sa/model.go: Added dns-account-01 to challenge type mappings

Testing:

core/*_test.go: Basic definition and validation tests
sa/sa_test.go: Database round-trip tests for dns-account-01 challenges

Dependencies:

Updated github.com/eggsampler/acme/v3 to release version v3.6.2

Next Steps: PR #8149 will add the PA/VA/RA validation logic and feature flags.

aarongable

Generally looks good. A few comments but nothing big or structural in the code itself.

That said, I do have one big/overarching comment:

I think it's an antipattern to introduce a feature flag but not immediately turn that feature flag on in the config-next integration tests configuration files. It means that we're not actually testing that flag in a meaningful way, and we could easily miss that step in a later PR (e.g. I don't see the flag being enabled in #8149 either).

But in this case, turning on the feature flag would immediately cause things to blow up, because the SA doesn't know how to convert a dns-account-01 challenge into a database entry.

This suggests to me that we're going about things in slightly the wrong order. In my opinion, the correct order to land these changes would be:

core definitions
SA/database support and WFE/rendering support, neither of which needs to be gated behind a flag
PA, VA, and RA support, plus the feature flag, and new integration tests

core/objects.go

policy/pa.go

features/features.go

go.mod

policy/pa_test.go

sheurich · 2025-05-02T19:06:12Z

Thanks @aarongable. I made the suggested changes and split the feature-flag + PA changes to a future PR.

sheurich · 2025-05-05T19:28:34Z

core definitions

SA/database support and WFE/rendering support, neither of which needs to be gated behind a flag

PA, VA, and RA support, plus the feature flag, and new integration tests

It seems that no changes are required for WFE rendering.

Given that the SA changes are minor (limited to updating the challenge type map), it makes sense to bundle the first two items into this PR. By including the necessary SA changes here, the remaining items—PA, VA, RA support, feature flag implementation, and integration tests—can be moved to #8149.

core/objects.go

sa/model.go

sheurich · 2025-05-28T18:29:18Z

@aarongable everything is up-to-date and ready for a final review.

sheurich · 2025-06-19T17:18:26Z

@aarongable Thanks for the detailed review! I've addressed all the feedback from your reviews:

✅ All Review Comments Resolved:

Code duplication in core/objects.go - Fixed with single case statement
SA database testing - Added comprehensive round-trip tests in sa/sa_test.go
Code formatting - Switch cases now on single lines as suggested
PA/feature flag separation - Moved to PR feat: Support for dns-account-01 Challenge #8149 as recommended

✅ Architectural Changes Implemented:
Following your guidance, this PR now contains only core definitions + SA support:

Core type definitions (core/objects.go, core/challenges.go)
SA model mappings (sa/model.go)
Associated tests with full coverage

The PA, VA, RA support + feature flags are in the follow-up PR #8149.

✅ Dependency Update:
Per your comment: "If you update this PR to be only the core definitions... then I think including this here will be fine" - the acme/v3 bump is included as conditionally approved.

This should be ready for final approval! 🚀

- Add `ChallengeTypeDNSAccount01` constant, `IsValid` update, and `RecordsSane` logic in `core/objects.go` - Add `DNSAccountChallenge01` function and handling in `core/challenges.go` - Add tests for the new challenge type in `core/core_test.go` and `core/objects_test.go` Implements core components for draft-ietf-acme-dns-account-label-00

- Add dns-account-01 challenge type to challTypeToUint map - Add dns-account-01 challenge type to uintToChallType map

…tions2 Extends the existing TestGetValidAuthorizations2 function to verify that authorizations with dns-account-01 challenges can be properly stored in and retrieved from the database.

danjeffery · 2025-07-21T15:49:34Z

This needs to be rebased to incorporate recent changes, but we believe it's otherwise complete. Any chance y'all can look it over this week? We should be able to work on the rebase Wednesday 07-23.

aarongable

LGTM with one formatting nit

sa/sa_test.go

empty line between stanzas Co-authored-by: Aaron Gable <aaron@aarongable.com>

aarongable · 2025-07-29T16:24:27Z

FYI, you don't have to push a merge commit just because the UI says "this branch is out of date". It'll clean that up on its own when we land the PR. We only require a merge commit when there are actual merge conflicts.

This pull request introduces support for the `dns-account-01` challenge type as specified in draft-ietf-acme-dns-account-label-01 (https://datatracker.ietf.org/doc/draft-ietf-acme-dns-account-label/01/), building upon PR #8140 which introduced the core type definitions. Core Implementation: - The policy engine in `policy/pa.go` is updated to offer the `dns-account-01` challenge for both standard and wildcard domains. - The main validation authority logic in `va/va.go` is updated to recognize `dns-account-01` challenges and route them to the correct validation routine, passing the necessary account information. - The core validation logic for `dns-account-01` is implemented in `va/dns.go`, which calculates the account-specific DNS label and verifies the corresponding TXT record. Configuration: - The `PAConfig` is updated to recognize `dns-account-01` as a valid challenge type which can be enabled in the PA config. - A new `DNSAccount01Enabled` feature flag is introduced in `features/features.go`. If this flag is not set, then the PA will not offer the new challenge type, and the VA will refuse to validate this challenge type. Testing: - A new suite of integration tests (`test/integration/dns_account_01_test.go`) has been added to cover various scenarios, including successful validation, incorrect TXT records, and wildcard domains. - The PA unit tests have been expanded to cover cases where the `dns-account-01` feature is both enabled and disabled. - The VA unit tests now include `va/dns_account_test.go`, specifically targeting the `dns-account-01` validation logic. - The mock DNS client (`bdns/mocks.go`) has been updated to simulate various `dns-account-01` responses. - The challenge test server client (`test/chall-test-srv-client/client.go`) now includes methods for adding and removing `dns-account-01` challenge responses. These changes provide a complete implementation of the `dns-account-01` challenge, including the necessary logic, configuration, and comprehensive testing to ensure its correctness and reliability.

sheurich requested a review from a team as a code owner April 23, 2025 20:37

sheurich requested a review from aarongable April 23, 2025 20:37

sheurich force-pushed the feat-dns-account-01-core branch 2 times, most recently from bd2501a to 217b809 Compare April 25, 2025 23:04

sheurich changed the title ~~feat: Add core definitions for dns-account-01 challenge type~~ feat: Add core definitions and PA offering for dns-account-01 Apr 25, 2025

sheurich force-pushed the feat-dns-account-01-core branch from 217b809 to e8b0275 Compare April 28, 2025 20:58

This was referenced Apr 28, 2025

feat(va, ra, sa): Implement dns-account-01 validation logic and protocol updates sheurich/boulder#4

Closed

feat: Support for dns-account-01 Challenge #8149

Merged

sheurich force-pushed the feat-dns-account-01-core branch from e8b0275 to 5b582d2 Compare May 1, 2025 16:43

aarongable requested changes May 1, 2025

View reviewed changes

core/objects.go Outdated Show resolved Hide resolved

policy/pa.go Outdated Show resolved Hide resolved

features/features.go Outdated Show resolved Hide resolved

go.mod Show resolved Hide resolved

policy/pa_test.go Outdated Show resolved Hide resolved

sheurich force-pushed the feat-dns-account-01-core branch from 5b582d2 to a813c3a Compare May 2, 2025 18:50

sheurich changed the title ~~feat: Add core definitions and PA offering for dns-account-01~~ feat: Add core definitions for dns-account-01 May 2, 2025

sheurich requested a review from aarongable May 2, 2025 19:52

sheurich force-pushed the feat-dns-account-01-core branch 4 times, most recently from 7e79102 to 70d50ce Compare May 8, 2025 19:35

aarongable reviewed May 9, 2025

View reviewed changes

core/objects.go Outdated Show resolved Hide resolved

sa/model.go Show resolved Hide resolved

aarongable requested review from a team and beautifulentropy and removed request for a team May 9, 2025 18:50

sheurich force-pushed the feat-dns-account-01-core branch 2 times, most recently from c00aeee to afc4313 Compare May 12, 2025 16:19

sheurich force-pushed the feat-dns-account-01-core branch 2 times, most recently from 4aafca3 to afd88db Compare May 28, 2025 18:11

sheurich requested a review from aarongable May 28, 2025 18:25

sheurich force-pushed the feat-dns-account-01-core branch from afd88db to 69ae9c0 Compare June 3, 2025 16:58

sheurich force-pushed the feat-dns-account-01-core branch from 69ae9c0 to e8ee04a Compare June 6, 2025 18:36

sheurich force-pushed the feat-dns-account-01-core branch from e8ee04a to 1c345a9 Compare June 17, 2025 18:38

sheurich force-pushed the feat-dns-account-01-core branch from d65b13d to 135b84a Compare June 19, 2025 19:16

sheurich mentioned this pull request Jun 20, 2025

feat: Support for dns-account-01 Challenge Type sheurich/boulder#8

Closed

sheurich force-pushed the feat-dns-account-01-core branch from 135b84a to 32de32b Compare June 24, 2025 17:48

sheurich added 4 commits June 24, 2025 14:17

chore: Update vendor dependency github.com/eggsampler/acme/v3 v3.6.2

3f0ddb9

feat: Add dns-account-01 challenge type to SA model

60e6787

- Add dns-account-01 challenge type to challTypeToUint map - Add dns-account-01 challenge type to uintToChallType map

test: Add dns-account-01 challenge test case to TestGetValidAuthoriza…

6726be6

…tions2 Extends the existing TestGetValidAuthorizations2 function to verify that authorizations with dns-account-01 challenges can be properly stored in and retrieved from the database.

sheurich force-pushed the feat-dns-account-01-core branch from 32de32b to 6726be6 Compare June 24, 2025 18:33

sheurich added 2 commits July 23, 2025 18:13

Merge branch 'main' into feat-dns-account-01-core

0210f6b

Merge branch 'main' into feat-dns-account-01-core

bc00ba8

aarongable previously approved these changes Jul 28, 2025

View reviewed changes

sa/sa_test.go Show resolved Hide resolved

aarongable requested a review from jsha July 28, 2025 22:44

Update sa/sa_test.go

ba43af2

empty line between stanzas Co-authored-by: Aaron Gable <aaron@aarongable.com>

sheurich dismissed aarongable’s stale review via ba43af2 July 28, 2025 23:46

aarongable approved these changes Jul 29, 2025

View reviewed changes

jsha approved these changes Jul 29, 2025

View reviewed changes

Merge branch 'main' into feat-dns-account-01-core

c37790c

aarongable merged commit 473b405 into letsencrypt:main Jul 29, 2025
12 checks passed

sheurich mentioned this pull request Sep 24, 2025

Add dns-account-01 challenge support unixcharles/acme-client#256

Merged

Uh oh!

Conversation

sheurich commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Core Definitions & Logic:

Storage Authority (SA) Support:

Testing:

Dependencies:

Uh oh!

aarongable left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sheurich commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sheurich commented May 5, 2025

Uh oh!

Uh oh!

Uh oh!

sheurich commented May 28, 2025

Uh oh!

sheurich commented Jun 19, 2025

Uh oh!

danjeffery commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aarongable left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aarongable commented Jul 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sheurich commented Apr 23, 2025 •

edited

Loading

sheurich commented May 2, 2025 •

edited

Loading

danjeffery commented Jul 21, 2025 •

edited

Loading